Update key vault data sdk to T2, mangement SDK to latest. by wantedfast · Pull Request #3 · Azure-Samples/key-vault-dotnet-recovery

wantedfast · 2020-05-14T06:09:46Z

Purpose

Update key vault data sdk to T2, key vault management SDK to latest.

…" to the value read from app.config. AzureEnvironment.AzureGlobalCloud remains as default.

FrankieTF · 2020-06-28T06:57:07Z

-        /// <summary>
-        /// Generic ADAL Authentication callback
-        /// </summary>
-        public static async Task<string> AcquireTokenAsync(string authority, string resource, string scope)


To track 2, there is new SecretClient instead of KeyVaultClient to use. this AcquireTokenAsync() is used to create KeyVaultClient in KeyvaultSampleBase delegate. So this is decided to remove

FrankieTF · 2020-06-28T07:02:14Z

-            // instantiate the data client
-            DataClient = new KeyVaultClient(ClientContext.AcquireTokenAsync);
+            //Get current Authority Host for Azure
+            TokenCredentialOptions options = new TokenCredentialOptions();


if we did not set options.AuthorityHost, ClientSecretCredential will always set as default "AzureGlobalCloud"

jongio · 2020-08-08T12:33:36Z

Please try with the new preview mgmt libs: https://www.nuget.org/packages/Azure.ResourceManager.KeyVault/1.0.0-preview.1

FrankieTF · 2020-09-17T03:13:42Z

Service: KeyVault; SDK: key vault mgmt sdk and azure key vault data sdk(t1)
Updates as belows:

Update key vault mgmt sdk and data sdk to track2.
Simplify ClientContext.
Update README.

jongio · 2020-09-22T16:58:42Z

@heaths - Can you please review?
@nickzhums - Do we want to update to the new mgmt libs?

heaths

Needs a few changes, but otherwise LGTM.

heaths · 2020-09-22T18:34:37Z

@@ -13,8 +10,6 @@ namespace AzureKeyVaultRecoverySamples
    /// </summary>
    public sealed class ClientContext


None of this class is really used anymore. Perhaps just replace it with the DefaultAzureCredential directly in the client like we normally do in samples. I worry the unnecessary complexity here will just make people copy the design when simply passing a DefaultAzureCredential is so much simpler.

heaths · 2020-09-22T18:36:55Z

+
+                // wait for purge complete
+                Console.Write("Waiting for Purging complete...");
+                Thread.Sleep(60 * 1000);


While sleeping for a bit is a good idea (and there's nothing more we can do anyway), you should actually have a limited retry loop. Purging can take a while, and will vary (it may be a scheduled task in the backend).

@heaths , i am not sure how to implement this limited retry loop. As i think there's nothing more we can do here.

heaths · 2020-09-22T18:39:02Z


                Console.Write("Retrieving newly created vault...");
-                var retrievedVault = await sample.ManagementClient.Vaults.GetAsync(rgName, vaultName).ConfigureAwait(false);
+                var retrievedVault = (await sample.ManagementClient.Vaults.GetAsync(rgName, vaultName).ConfigureAwait(false)).Value;


Like the sample above, I'd stick with Response<T> here and use its Value property only when needed.

heaths · 2020-09-22T18:39:30Z

        /// </summary>
        protected ClientContext context;
-
+                


Nit: git doesn't like whitespace-only lines.

heaths · 2020-09-22T18:41:04Z

+- dotnet-core
 description: "This repo contains sample code demonstrating the backup/restore and recoverable deletion functionality of Azure Key Vault using the Azure .NET SDK."
-urlFragment: net-sdk-samples
+urlFragment: key-vault-dotnet-recovery


This will result in broken links. urlFragment should neve be changed. It's unfortunate it was generic before, but should remain the same. Just be more specific going forward.

heaths · 2020-09-22T18:41:40Z

+To complete this tutorial:

-The recoverable deletion functionality is also referred to as 'soft delete'; consequently, a permanent, irrecoverable deletion is referred to as 'purge'.
+* Install .NET Core 2.0 version for [Linux] or [Windows]


2.0 is out of support. 2.1 is LTS, but so is 3.1. We should at least recommend an LTS.

heaths · 2020-09-22T18:42:04Z

+### Create an App registration using the Azure Portal

-## Getting Started
+*  Go to the [Azure Portal] and log in using your Azure account. 


Steps should be numbered. Alternatives should be bulleted.

heaths · 2020-09-22T18:43:47Z

+[free account]: https://azure.microsoft.com/free/?WT.mc_id=A261C142F
+[Azure Portal]: https://portal.azure.com
+[Recovery scenario samples for Azure Key Vault using the Azure Node SDK]: https://docs.microsoft.com/en-us/samples/azure-samples/key-vault-node-recovery/key-vault-node-recovery/
+[Azure Key Vault documentation]: https://docs.microsoft.com/en-us/azure/key-vault/general/basic-concepts


URLs should not include the locale. In VSCode, install the Microsoft Docs Authoring Pack for rules you should follow when writing markdowns (it would catch this issue).

I see a couple URLs like that. This always goes to the specified language/locale, while leaving it out uses the browsers' preferred languages (better for non-en-US readers).

heaths · 2020-09-22T18:46:00Z

Happy to review, but @jongio or @wantedfast, could I get a review on my sample at Azure/azure-sdk-for-net#15123? It's been sitting there for quite a while. The brunt of it was previously reviewed by Alex at its original home (https://github.com/heaths/KeyVaultProxy), but I had to make a few changes to build / run in our repo, per @weshaggard's desire to host samples in a single place going forward.

nickzhums · 2020-09-23T02:55:25Z

@wantedfast @heaths just FYI - .NET SDK team is designing a new set of APIs that will be previewed at the end of year. There can be some upcoming changes in this key vault code sample. The Track 2 .NET SDK won't be GA-ed for a while, so if we expect the customer to see a stable version in a code sample, probably better to stay on Track 1, otherwise, feel free to update to Track 2.
cc @jongio

heaths · 2020-09-24T00:43:27Z

The preview functionality coming out later is in addition to what is here. We're aren't changing anything that is demo'd here. Seems better to show customers more track 2 code since we are deprecating track 1.

heaths · 2020-10-09T16:59:34Z

If possible, please avoid force-pushing until you're ready to merge (for example, if rebasing on master to pick up recent changes). I can't tell what's changed so I will have to re-review the entire PR again.

heaths · 2020-10-09T17:00:57Z

-            catch (CloudException ce)
            {
-                Console.WriteLine("Unexpected ARM exception encountered: {0}", ce.Message);
+                Console.WriteLine("Unexpected KeyVault exception encountered: {0}", ex.Message);


Nit: the product name is "Key Vault" (with a space). We should use that in customer-facing messages / docs.

heaths · 2020-10-09T17:01:49Z

        /// </summary>
        protected ClientContext context;
-
+        


Nit: extraneous whitespace.

heaths · 2020-10-09T17:02:43Z

+To complete this tutorial:

-The recoverable deletion functionality is also referred to as 'soft delete'; consequently, a permanent, irrecoverable deletion is referred to as 'purge'.
+* Install .NET Core 3.1 version for [Linux] or [Windows]


Use - consistently throughout the markdown (you use mainly this below).

U mentioned steps need numbered, alternative should be bulleted. I think this is much better than mix them.
Do you still think we should keep consistency or changing below with numbered steps?
Like what i have done now.

FrankieTF · 2020-10-10T03:28:33Z

Sorry for let you review again, this force-pushing is because this PR needs step back. I will be more careful with force-pushing next time.

wantedfast added 2 commits May 14, 2020 14:06

Update key vault data sdk to T2, mangement SDK to latest.

6258ce0

Update the readme

7556d34

wantedfast marked this pull request as ready for review May 18, 2020 07:33

update passing value from constant "AzureEnvironment.AzureGlobalCloud…

33c0a27

…" to the value read from app.config. AzureEnvironment.AzureGlobalCloud remains as default.

FrankieTF reviewed Jun 28, 2020

View reviewed changes

Frank Li (Wicresoft North America Ltd) added 4 commits June 30, 2020 13:41

Update key vault data sdk to T2

b4fe0e5

little change for string vault name

af0fa53

remove redundant blank line and values

799a33b

Replace get creadential method into ClientContext class.

ee8649e

heaths approved these changes Sep 22, 2020

View reviewed changes

Removing mgmt libs, keep data SDK.

f406984

FrankieTF force-pushed the Dev-updateSDK branch from e21e18a to f406984 Compare October 9, 2020 07:15

heaths approved these changes Oct 9, 2020

View reviewed changes

Frank Li (Wicresoft North America Ltd) added 2 commits October 10, 2020 11:18

Updated README.

6fd22be

Found another KeyVault should be seperated.

eb181a1

jongio merged commit 589d10a into Azure-Samples:master Nov 18, 2020

		@@ -13,8 +10,6 @@ namespace AzureKeyVaultRecoverySamples
		/// </summary>
		public sealed class ClientContext

Conversation

wantedfast commented May 14, 2020

Purpose

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jongio commented Aug 8, 2020

Uh oh!

FrankieTF commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jongio commented Sep 22, 2020

Uh oh!

heaths left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heaths commented Sep 22, 2020

Uh oh!

nickzhums commented Sep 23, 2020

Uh oh!

heaths commented Sep 24, 2020

Uh oh!

heaths commented Oct 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankieTF commented Oct 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

FrankieTF commented Sep 17, 2020 •

edited

Loading